Repository navigation
Conversation
f93aa25 to
7b35032
Compare
There was a problem hiding this comment.
@lucasfang, I left some comments for this pull request. PTAL.
There was a problem hiding this comment.
@lucasfang , one additional correctness issue remains in the direct system-table read path.
zjw1111
left a comment
There was a problem hiding this comment.
Two correctness issues remain in the table-first write/commit path at 13fba3780476ce4177d2197e0d7f62816bdd3047.
zjw1111
left a comment
There was a problem hiding this comment.
One branch identity issue remains in SwitchToBranch() at 13fba3780476ce4177d2197e0d7f62816bdd3047.
There was a problem hiding this comment.
@lucasfang, thanks for updates. Only one fallback-read correctness issue comment. PTAL.
There was a problem hiding this comment.
@lucasfang, thanks for updates. I left three comments for correctness issues remaining at ba063c6: explicit context branch validation, dynamic options for system-table views, and accumulated overrides when copying to another branch.
| PAIMON_RETURN_NOT_OK(CheckContextNamesThisTable("write", ctx->GetRootPath(), | ||
| ctx->GetIdentifier(), | ||
| /*context_table_id=*/std::nullopt)); |
There was a problem hiding this comment.
Reject explicit context branches that differ from the table. A main-branch table accepts a WriteContext built with WithBranch("dev") because this validation checks only the path and identifier. NewCommit() similarly accepts AddOption(Options::BRANCH, "dev") when the schema has no branch option. Both operators then use the store's main-branch managers, so a write intended for dev can be published to main without an error. Please validate explicitly supplied branch settings against CurrentBranch() before constructing either operator, and cover this case through the table-based write and commit entry points.
| Result<std::shared_ptr<Table>> SystemTable::Copy( | ||
| const std::map<std::string, std::string>& /*dynamic_options*/) const { | ||
| return Status::NotImplemented(Name(), " system table is read-only: copying is not supported"); |
There was a problem hiding this comment.
Preserve dynamic scan options for system-table views. Path-based factories now reject system tables, but this unconditional rejection also prevents configuring their replacement catalog-loaded handles. For example, NewScan() on tbl$ro ignores context options scan.mode=from-snapshot and scan.snapshot-id, using only the options captured when the table was loaded; Copy() cannot apply them either. A historical scan consequently reads the latest snapshot. Please preserve a supported way to apply dynamic options to read_optimized and audit_log views, and test the selected snapshot through Catalog::GetTable().
| PAIMON_RETURN_NOT_OK(SchemaValidation::CheckImmutableOptions(*base_schema, overridable)); | ||
| PAIMON_ASSIGN_OR_RAISE(std::unique_ptr<TableSchema> merged_schema, | ||
| base_schema->MergeOptions(schema_overridable)); | ||
| std::map<std::string, std::string> accumulated = dynamic_options_; | ||
| accumulated.erase(Options::TYPE); | ||
| accumulated.erase(Options::BRANCH); | ||
| for (const auto& [key, value] : overridable) { | ||
| accumulated.insert_or_assign(key, value); |
There was a problem hiding this comment.
Merge accumulated overrides into a branch copy's schema. When Copy() switches branches, base_schema becomes the target branch's persisted schema, but MergeOptions() receives only this call's overrides. Earlier dynamic_options_ are retained in accumulated without reaching the new store's CoreOptions. For example, Copy({{Options::KEY_VALUE_SEQUENCE_NUMBER_ENABLED, "true"}}) followed by Copy({{Options::BRANCH, "dev"}}) loses sequence-number support when dev omits that setting, causing an _SEQUENCE_NUMBER projection to fail even though option validation still considers the setting enabled. Please merge the accumulated overrides consistently into the target schema and add a regression test for successive copies.
Purpose
Linked issue: close #459
The single
Tableclass dispatched managed, format, and system tables throughifbranches in one 1400-linetable.cpp; every operator call re-merged an options map and re-parsedCoreOptions(Java'sAbstractFileStoreparses once), and the served file system was resolved before the schema's access configuration was known. This splits the hierarchy the way Java splits it and gives context options one clear place to speak, fixing the defects that the ambiguity hid.Changes:
Table(include/paimon/catalog/table.h) becomes an abstract base:Name/FullName/NewXxx/Copyare virtual, the constructor is protected. Implemented byFileStoreTable(withAppendOnlyFileStoreTable/PrimaryKeyFileStoreTablesubclasses,src/paimon/core/table/), byFormatTable(now deriving fromTable, owning its own assembly —format_table_loaderis absorbed), and by theSystemTablefamily.FileStore(src/paimon/core/table/file_store.h/.cpp), the counterpart of Java'sAbstractFileStore: materializesCoreOptionsonce through astatic Create(), resolves the served file system after the options are known (schema-carried access configuration names its own backend; the caller's explicit file system wins over it), carries the file-system scheme map, and hosts the operator factories.FileStoreTableholds its schema/snapshot managers and its branch;Copy()hands the managers over unless a file-system option among the copy's changes the resolution, and never half-appliestypeorbranch;SwitchToBranch()derives a table of another branch.TableScan::Create(ctx)etc.) merges them over the schema's throughTable::Copy()— a table is the product of its load — whileNewXxxon a held table refuses a contradicting option (option 'X' is 'A' in the context but 'B' on the table, values redacted throughSensitiveConfigUtils::RedactValue). The four context builders document the semantics.RestCatalog(src/paimon/rest/rest_catalog.cpp):GetTableserves the global system tables of thesysdatabase locally (they previously reached the managed-table load, which statically cast their read-only schema — undefined behavior), binds per-table system tables toGetTableFileSystem()so data-token credentials reach$snapshotsand friends, andFileStoreTable::Createrefuses a non-managed schema instead of casting it.read_optimized_system_table.cpp,audit_log_system_table.cpp): the underlying scans and reads prefer the context's file system and fall back to the table's own only when the context names neither a file system nor a scheme map (the previous unconditional fallback was refused when a scheme map was present). The scan-fallback read context merges the load's dynamic options, so the fallback reader recognises a sequence-number column the main one does.Tests
src/paimon/core/table/table_test.cpp(expanded): managers of the branch a table was loaded with,SwitchToBranch,Copykeeping that branch when handed abranchoption (both directions, verified to fail before the fix), operators refusing an option that contradicts a held table, a factory-path override of a schema-carried key, cache injection, thesys-database REST open, and the per-table file system.src/paimon/core/table/source/table_read_test.cpp(TestMergeOptions,TestDependencyReadWithoutCatalogUsesReadOptions): factory-path merging of context options into the served schema, and the dependency catalog context seeing the merged header value.src/paimon/core/catalog/file_system_catalog_test.cpp,src/paimon/rest/rest_catalog_test.cpp, and the inte tests (scan_and_read,realtime_write,read,scan,write_and_read_inte_with_index) adapted to theTableAPI.PAIMON_ENABLE_REST=ON):paimon-core-test2492/2492;paimon-rest-test152/152;ctest -j 9632/33 — the only failure ispaimon-write-and-read-inte-testwith its 2("parquet", "jindo")jindo-credential environment cases, unchanged from the baseline;paimon-data-evolution-table-test306/306 andpaimon-scan-and-read-inte-test511/511;pre-commit run --files <changed>passed (clang-format applied);git diff --checkclean.API and Format
No storage format or protocol change. Breaking public API changes:
Tableis now abstract (constructor protected,Name/FullName/NewXxx/Copyvirtual,Table::CurrentBranch()replaced byFileStoreTable::CurrentBranch());FormatTablederives fromTable(staticFormatTable::Copy(table, options)becomes the virtualTable::Copy(options),Location()becomesGetRootPath());FileStoreTable::SwitchToBranchis new. Observable semantics: schema options that do not parse fail at table-build time (previously at the first operator); a table without a schema-carried backend or a caller file system is served by the default local one instead of a null one; context options are merged at load time only.Documentation
No new user-facing feature; the option semantics on the four context builders are documented in the same change, and
docs/source/user_guide/format_table.rstfollows theFormatTableAPI rename.Generative AI tooling
Generated-by: Qoder